Handle non-Error objects thrown as error gracefully, still attempt to report them in a useful way #232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CherryDT
changed the title
vmarchaud
suggested changes
Mar 1, 2019
There was a problem hiding this comment.
Thanks for taking the time to fixing this yourself, overall i agree that it could be useful to be able to notify different kind of objects.
I choose to force it to be an error because we got few case before when error where not saved because of the wrong data structure which is hard to understand for customers generally.
… limit for those errors' messages, increase robustness against weird values
vmarchaud
suggested changes
Mar 3, 2019
vmarchaud
reviewed
Mar 3, 2019
vmarchaud
approved these changes
Mar 4, 2019
|
I will release this tomorrow under the 4.1.0 tag, thanks again for taking the time to change this ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some code (unfortunately) throws things that are not
Errorobjects. The things thrown may be raw data returned from an API, plain objects, strings, in the worst case evennullorundefined.While
notifyErrordid already reject things that were notErrorobjects, the other handlers (uncaught exception, unhandled rejection, Express/Koa middlewares) did not.I think that
a) The behavior should be the same regardless which handler handles the error
b) Things that are not
Errorobjects should still be reported in the most meaningful way (i.e. giving the developer enough information to understand what the error actually was).Therefore, I modified the behavior so that things other than
Errors are simply converted to anErrorwith the JSON representation of the old data asmessage.Even though the stack of those errors is then meaningless, the JSON data should give the developer enough information, definitely more than not seeing the error at all (old behavior of
notifyError), getting no description of the error (old behavior of the other handlers, sinceerr.messagemay simply be undefined) or crashing in a weird way (old behavior of the other handlers in the case that the error wasnullorundefined).